Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saved report bug fixes #5649

Merged
merged 5 commits into from
Jan 8, 2025
Merged

Conversation

gilluminate
Copy link
Contributor

@gilluminate gilluminate commented Jan 7, 2025

Closes HJ-357, HJ-155

Description Of Changes

  • Updates functionality of Custom Report reset to reset the entire table to its default state once applied, not just reset the filter.
  • Restores the Filter accordion label
  • Fixes ordering to be based on the tableinstance columns, rather than the backend defined columns.
  • Enhances Cypress tests to help catch these issues in the future

Steps to Confirm

  1. Clear Local Storage for AdminUI or open an ingocnito window. Log in again.
  2. Assuming you have systems set up in your Admin UI, visit the Datamap Report page /reporting/datamap.
  3. Add a filter to the report using the Filters dialog. While doing that, note that all 3 accordions have labels (the top one was missing).
  4. Update the grouping of the report. Note that the only column order change when doing so are the first 2 columns (the rest were being reordered unintentionally)
  5. Change various other aspects of the table (column order, column naming, column visibility) and save as a custom report using the reports dropdown.
  6. Reload the page, ensuring that the custom report is still applied.
  7. Open the custom report dropdown again and click "reset" then "Apply"
  8. The table should now be back to default state. Reloading the page should retain this default state.

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Followup issues:
    • Followup issues created (include link)
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

Copy link

vercel bot commented Jan 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Jan 8, 2025 5:27pm

Copy link

cypress bot commented Jan 7, 2025

fides    Run #11667

Run Properties:  status check passed Passed #11667  •  git commit c271d68abb ℹ️: Merge c05a7f7b66a7377e159a9235a5a9eebf96178c93 into b805c8c8d9f06578b49b4faaeab2...
Project fides
Branch Review refs/pull/5649/merge
Run status status check passed Passed #11667
Run duration 00m 51s
Commit git commit c271d68abb ℹ️: Merge c05a7f7b66a7377e159a9235a5a9eebf96178c93 into b805c8c8d9f06578b49b4faaeab2...
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

@gilluminate gilluminate force-pushed the gill/HJ-155/saved-report-reset-bug branch from 2610271 to f0b988e Compare January 7, 2025 17:28
Copy link
Contributor

@andres-torres-marroquin andres-torres-marroquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, looking forward to pull these changes into my current work.

@gilluminate gilluminate merged commit 2d9f1ef into main Jan 8, 2025
20 checks passed
@gilluminate gilluminate deleted the gill/HJ-155/saved-report-reset-bug branch January 8, 2025 17:49
Copy link

cypress bot commented Jan 8, 2025

fides    Run #11668

Run Properties:  status check passed Passed #11668  •  git commit 2d9f1ef270: Saved report bug fixes (#5649)
Project fides
Branch Review main
Run status status check passed Passed #11668
Run duration 00m 48s
Commit git commit 2d9f1ef270: Saved report bug fixes (#5649)
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants